Display error/reason to user, if a transport had to be restarted - #231
msirringhaus wants to merge 3 commits into
Conversation
b2209ab to
852e47b
Compare
iinuwa
left a comment
iinuwa
left a comment
There was a problem hiding this comment.
haven't run this yet, but a couple of questions
| } else { | ||
| // Pre-active: the QR was never consumed or the BLE channel | ||
| // failed before the phone responded. Reissue silently. | ||
| tracing::debug!(?err, "Hybrid pre-active error, reissuing QR silently"); |
There was a problem hiding this comment.
I'm reading this on the train, so just wanted to double-check: can this get us in a restart loop? Is there a point where we eventually stop retrying a transport (without failing the ceremony)?
There was a problem hiding this comment.
Yes, it could potentially end in a loop. USB has a somewhat similar problem, actually.
I actually have to think about this a bit more. If and how to display this to the user, for example (do we want something like "we removed hybrid from the list, because it failed too often" in the UI?), and how to structure the error handling to deal with this.
It's somewhat hard to distinguish between "this will never work"-errors and those who might be recoverable.
I need a clearer structure for stopping transports (e.g. probably makes sense to stop it on Internal-errors)
Or we never stop/remove a transport, but only throttle it with delays between retries?
| /// as invalid by `TryFrom`. | ||
| #[repr(u8)] | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub enum TransportRestartReason { |
There was a problem hiding this comment.
We're currently using PascalCase strings to communicate enums everywhere else in the API. I find that easier to read in D-Bus output while debugging, and the extra 10 or so bytes in the message doesn't hurt.
Do you have opinions either way? If not, I'd recommend to make this a string for consistency.
2 participants
Note: Based on #230 , and needs to be rebased if/when this lands.
Only send events, if the device was actively used by the user (Hybrid: QR scanned, USB: initial touch done, etc.).
Extended the Restarting-events introduced in #230 with an error reason. Display it at the bottom of the start page. Removed those error codes that trigger restarts from the terminal error code list.
Last commit updates the lang-files.